Skip to content

Conversation

@cigaly
Copy link
Contributor

@cigaly cigaly commented Nov 17, 2025

Jira issue HHH-19935 UuidVersion7Strategy can generate cosecutive UUIDs that are equal

Increment to current sequence number is properly generated so that it never can be zero.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


}
else {
final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFFL );
final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFEL ) + 1;
Copy link
Member

@beikov beikov Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the purpose of this code was exactly, so please be so kind and add some code comments that explain this a bit. Since the maximum value that can be returned by nextLong( 0xFFFF_FFFFL ) is 0xFFFF_FFFEL, I think this should be fine, no?

Suggested change
final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFEL ) + 1;
final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFFL ) + 1;

But more importantly, like I asked in the other PR, what aren't we just always using a random sequence? Citing my other comment from before here again:

Yes, please create a Jira issue and a PR. Also, while you're at that, can you please add a javadoc comment explaining the * 0.004096 in the nanos method?

I'm also a bit surprised about the 0xFFFF_FFFFL constant now that I'm looking at it again. The javadoc of the class mentions a 62 bit pseudo-random counter, but here we just add a 16 bit random value to an existing sequence. What is the reason for this again? We could always create the full random 62 bits, but I guess there is an advantage to this process?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, bound value should not be changed, only 1 should be added to generated random.

If we replace current

				final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFFL ) + 1;

with

final long nextSequence = Holder.numberGenerator.nextLong( 0x3FFF_FFFF_FFFF_FFFFL + 1 );

then

				return nextSequence > MAX_RANDOM_SEQUENCE
						? new State( lastTimestamp.plusNanos( 250 ), randomSequence() )
						: new State( lastTimestamp, nextSequence );

should be replaced with

				return lastSequence >= nextSequence
						? new State( lastTimestamp.plusNanos( 250 ), randomSequence() )
						: new State( lastTimestamp, nextSequence );

in order to ensure monotonicity. Condition lastSequence >= nextSequence will be true much oftern than nextSequence > MAX_RANDOM_SEQUENCE. This will cause nanos part of state to grow and not unlikely will have influence on millis part as well if large number of UUIDs is generated in very short period of time. If you think that this is OK, I can make changes above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble understanding why we are even bothering with all this. PostgreSQL for example always uses random 62 bits. Why can't we do that as well? Chances of a collision should be small to non-existent anyway AFAIU. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, here is version that will generate completely random 62 bits

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I would be interested though why you chose this somewhat more complicated path in the beginning though. Maybe I'm missing something?

@cigaly cigaly force-pushed the HHH-19935-UuidVersion7Strategy_can_generate_consecutive_UUIDs_that_are_equal branch 2 times, most recently from baaefee to 86ad13d Compare November 18, 2025 12:23
@cigaly cigaly force-pushed the HHH-19935-UuidVersion7Strategy_can_generate_consecutive_UUIDs_that_are_equal branch from 86ad13d to f789726 Compare November 18, 2025 12:29
Comment on lines +74 to +76
final long nextSequence = randomSequence();
return lastSequence >= nextSequence
? new State( lastTimestamp.plusNanos( 250 ), nextSequence )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the nextSequence must be greater than the lastSequence to guarantee monotonicity? And the addition of 250 is because that is the lowest sensible value to add due to using only the upper 12 bit of the nanos? Since the highest potential nano value 999_999 has 20 bits which, I would assume that the smallest sensible value to add is 2^8 i.e. 256. Can you please explain the reasoning in a code comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants